Refactor block-cipher packet-length probing to avoid unsafe state duplication - #706
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors block-cipher packet-length probing to replace unsafe cipher state duplication with an explicit internal probe trait, preserving cipher state isolation during SSH packet reads.
Changes:
- Adds
PacketLengthProbeand wires it into block cipher opening keys. - Implements cloned-state probing for stream/CTR ciphers and CBC decryptors.
- Adds regression tests for probe state isolation and end-to-end read behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
russh/src/cipher/block.rs |
Adds the probe trait, updates opening key bounds, replaces unsafe probing, and adds stream/read-path tests. |
russh/src/cipher/cbc.rs |
Adds CBC-specific packet-length probing and CBC state-isolation test coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
as usual this turned out to be more complicated; either bumping versions is needed or a bunch more stuff to get perf parity. |
|
I pushed cargo minimal-versions check --all-features --no-dev-depsI also tested the alternative that avoids changing the dependency floor: wrap CTR stream ciphers so packet-length probing can recreate a fresh cipher from the original key/IV instead of requiring Packet-read Criterion comparison, If the diff --git a/russh/src/cipher/block.rs b/russh/src/cipher/block.rs
index dc9c02e542649ae30bd9b4df51a792a7ce32cda1..fe2debeb6258f83f9a916489638894731bf01a90 100644
--- a/russh/src/cipher/block.rs
+++ b/russh/src/cipher/block.rs
@@ -228,9 +228,50 @@ impl<T: StreamCipher> BlockStreamCipher for T {
}
}
-impl<T: StreamCipher + Clone> PacketLengthProbe for T {
+pub(crate) struct ProbeableStreamCipher<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> {
+ cipher: C,
+ key: GenericArray_0_14<u8, C::KeySize>,
+ iv: GenericArray_0_14<u8, C::IvSize>,
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> KeySizeUser for ProbeableStreamCipher<C> {
+ type KeySize = C::KeySize;
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> IvSizeUser for ProbeableStreamCipher<C> {
+ type IvSize = C::IvSize;
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> KeyIvInit for ProbeableStreamCipher<C> {
+ fn new(
+ key: &GenericArray_0_14<u8, Self::KeySize>,
+ iv: &GenericArray_0_14<u8, Self::IvSize>,
+ ) -> Self {
+ Self {
+ cipher: C::new(key, iv),
+ key: key.clone(),
+ iv: iv.clone(),
+ }
+ }
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> BlockStreamCipher
+ for ProbeableStreamCipher<C>
+{
+ fn encrypt_data(&mut self, data: &mut [u8]) {
+ self.cipher.apply_keystream(data);
+ }
+
+ fn decrypt_data(&mut self, data: &mut [u8]) {
+ self.cipher.apply_keystream(data);
+ }
+}
+
+impl<C: StreamCipher + KeyIvInit + KeySizeUser + IvSizeUser> PacketLengthProbe
+ for ProbeableStreamCipher<C>
+{
fn decrypt_packet_length_block(&self, first_block: &mut [u8; 16]) {
- let mut cipher = self.clone();
+ let mut cipher = C::new(&self.key, &self.iv);
cipher.apply_keystream(first_block);
}
}
@@ -245,7 +286,7 @@ mod tests {
use digest::typenum::U16;
use tokio::io::AsyncWriteExt;
- use super::{BlockStreamCipher, OpeningKey, PacketLengthProbe};
+ use super::{BlockStreamCipher, OpeningKey, PacketLengthProbe, ProbeableStreamCipher};
use crate::mac::MacAlgorithm;
use crate::sshbuffer::SSHBuffer;
@@ -259,7 +300,7 @@ mod tests {
let mut ciphertext = plaintext;
encryptor.apply_keystream(&mut ciphertext);
- let cipher = Ctr128BE::<Aes128>::new(&key.into(), &iv.into());
+ let cipher = ProbeableStreamCipher::<Ctr128BE<Aes128>>::new(&key.into(), &iv.into());
let mut probed_block = ciphertext;
cipher.decrypt_packet_length_block(&mut probed_block);
assert_eq!(probed_block, plaintext);
diff --git a/russh/src/cipher/mod.rs b/russh/src/cipher/mod.rs
index 1b10055b8b165e70e6f94bab201885266150502b..1fd4e7b7e11ce394ccf9fd0e4853874e830b0e3a 100644
--- a/russh/src/cipher/mod.rs
+++ b/russh/src/cipher/mod.rs
@@ -45,7 +45,7 @@ pub(crate) mod chacha20poly1305;
pub(crate) mod clear;
pub(crate) mod gcm;
-use block::SshBlockCipher;
+use block::{ProbeableStreamCipher, SshBlockCipher};
use chacha20poly1305::SshChacha20Poly1305Cipher;
use clear::Clear;
use gcm::GcmCipher;
@@ -103,9 +103,12 @@ pub const NONE: Name = Name("none");
pub(crate) static _CLEAR: Clear = Clear {};
#[cfg(feature = "des")]
static _3DES_CBC: SshBlockCipher<CbcWrapper<des::TdesEde3>> = SshBlockCipher(PhantomData);
-static _AES_128_CTR: SshBlockCipher<Ctr128BE<Aes128>> = SshBlockCipher(PhantomData);
-static _AES_192_CTR: SshBlockCipher<Ctr128BE<Aes192>> = SshBlockCipher(PhantomData);
-static _AES_256_CTR: SshBlockCipher<Ctr128BE<Aes256>> = SshBlockCipher(PhantomData);
+static _AES_128_CTR: SshBlockCipher<ProbeableStreamCipher<Ctr128BE<Aes128>>> =
+ SshBlockCipher(PhantomData);
+static _AES_192_CTR: SshBlockCipher<ProbeableStreamCipher<Ctr128BE<Aes192>>> =
+ SshBlockCipher(PhantomData);
+static _AES_256_CTR: SshBlockCipher<ProbeableStreamCipher<Ctr128BE<Aes256>>> =
+ SshBlockCipher(PhantomData);
static _AES_128_GCM: GcmCipher = GcmCipher(&ALGORITHM_AES_128_GCM);
static _AES_256_GCM: GcmCipher = GcmCipher(&ALGORITHM_AES_256_GCM);
static _AES_128_CBC: SshBlockCipher<CbcWrapper<Aes128>> = SshBlockCipher(PhantomData); |
e26ab2f to
f61e779
Compare
f61e779 to
e3d63e3
Compare
|
Thank you! As a baseline, minimum dependency bumps are always acceptable by default as long as it's for a stable version. |
Summary
PacketLengthProbetraitMinimum fix
The commit
7895b84fix block cipher packet length state cloning is the smallest self-contained fix.This PR keeps the same behavior but refactors the probing path so CBC does not need to clone the full wrapper on every packet-length probe.
Why this refactor
OpeningKey::decrypt_packet_length()needs to decrypt the first block without advancing the real cipher state.Before the fix, it did that with an unsafe bitwise copy of
self.cipher, which is unsound for ownership-bearing cipher state.The direct fix was to clone the cipher state before probing. That is correct, but it adds avoidable overhead on the packet read path.
The refactor makes the probe behavior explicit:
That keeps the fix sound while preserving the packet-read performance characteristics much more closely.
Performance
I compared the end-to-end packet-read path three ways with the same Criterion bench and
--sample-size 10:mainThe clone-based fix is substantially slower than
main, roughly 4x to 5x on these packet-read cases.This refactor brings performance back in line with
main:aes256-ctr, payload11:main378-385ns, clone fix1.698-1.719us, this PR366-377nsaes256-ctr, payload100:main411-431ns, clone fix1.730-1.776us, this PR372-386nsaes256-ctr, payload1000:main558-576ns, clone fix1.910-1.966us, this PR551-579nsaes256-cbc, payload11:main413-425ns, clone fix1.724-1.765us, this PR400-413nsaes256-cbc, payload100:main429-440ns, clone fix1.956-2.012us, this PR402-430nsaes256-cbc, payload1000:main749-787ns, clone fix2.293-2.446us, this PR758-771nsOpenSSH reference point
This matches the same basic invariant OpenSSH relies on in its packet read path: decrypt enough state to determine the packet length without corrupting the real transport cipher state used for the full packet.
Testing
decrypt_packet_length_uses_independent_cipher_statecrashes onmain, which is the regression this change fixes.On this branch:
cargo test -p russh stream_cipher_probe_does_not_advance_cipher_state -- --nocapturecargo test -p russh decrypt_packet_length_uses_independent_cipher_state -- --nocapturecargo test -p russh packet_length_probe_does_not_advance_cbc_decryptor_state -- --nocapturecargo check -p russh --lib